Skip to content

Conversation

angelosilvestre
Copy link
Collaborator

[SuperEditor] Don't activate an action tag when placing the caret at an existing tag pattern. (Resolves #2392)

Steps to reproduce

  • Initialize the editor with a document containing a tag pattern, for example, 'This is origin/main branch"
  • Place the caret at the word after the "/"

Actual result

The editor starts composing an action tag.

Expected result

The editor shouldn't start composing an action tag.

The issue is that the tag reaction only checks if there is a tag around the caret. It does not check if the user is actually modifying the document.

This PR modifies the action tag reaction to ensure that there is at least an edit in the change list before trying to start composing a tag.

While testing, I found another behavior that I'm not sure is intended. When moving the caret within an existing tag, the tag composition is updated:

Screen.Recording.2025-08-16.at.16.46.32.mov

I kept this behavior and added a test for that. If this isn't intended, then I can remove this behavior and update the test.

range: SpanRange(tagAroundPosition.indexedTag.startOffset, tagAroundPosition.indexedTag.endOffset),
)
.isNotEmpty;
if (changeList.none((event) => event is DocumentEdit) && !hasComposingTagAttribution) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we wait so long to do this check? Can't we do this check at the very beginning before all this other stuff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can move it to the beginning, because we need to let the code that clears the attributions when the user presses ESC run.

return;
}

final hasComposingTagAttribution = textNode!.text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not already checking for an existing composing tag around the user's selection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We check if there is a text that matches a tag pattern, but not if it is already attributed.

expect(tagNotificationCount, 7);
});

testWidgetsOnAllPlatforms("does not start composing when placing the caret at an existing tag pattern",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is what the original ticket was about, was it? Isn't the issue about the surrounding text, not the user taping to place the caret?

Your example here says "origin/main" - but even if the user was typing, do we expect the "/" to trigger an action tag with "origin" immediately upstream? Maybe we want that for action tags, but I know we don't for "@" mentions or "#" tags....

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is what the original ticket was about, was it? Isn't the issue about the surrounding text, not the user taping to place the caret?

No, that was other ticket. This ticket is about initializing the editor with an existing document that contains text that matches the tag pattern. Tapping at such text shouldn't start a tag composition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SuperEditor] - Action tag activating on pre existing tag rule
2 participants